Add memory accounting to exponential histogram library. #132580
Conversation
JonasKunz
added
>non-issue
:StorageEngine/Mapping
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
...stogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramMerger.java
Outdated
Show resolved
Hide resolved
|
|
||
| import org.apache.lucene.util.RamUsageEstimator; | ||
|
|
||
| class RamEstimationUtil { |
There was a problem hiding this comment.
Should these be part of RamUsageEstimator? This can happen in a follow-up, or separate PR.
There was a problem hiding this comment.
Would be nice, but RamUsageEstimator is part of lucene-core. I don't know if Lucene folks would want this as part of their public API?
T-Digest btw also implements this methods itself, but I though it is not worth it to extract them in some shared libary.
There was a problem hiding this comment.
I see. Yeah moving this to a shared util for use inside ES makes sense, you can follow-up on that.
There was a problem hiding this comment.
@martijnvg this was the utility we talked about in the standup.
One idea was to move it into libs:core, but that would imply that libs:core then needs to depend on lucene-core.
If you have a suggestion on a good place for this utility or think keeping it copied as-is it would be great to hear.
...stogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialScaleUtilsTests.java
Outdated
Show resolved
Hide resolved
…onentialhistogram/ExponentialHistogramMerger.java Co-authored-by: Kostas Krikellas <[email protected]>
--------- Co-authored-by: elasticsearchmachine <[email protected]> Co-authored-by: Kostas Krikellas <[email protected]>
Implements memory accounting (
Accountable) and circuit breaker support for the exponential histogram library added in #131220. Just like the T-Digest library, the circuit breaker is abstracted away through an interface, so that no direct dependency on it is required for the lib.This PR now defines for the
ExponentialHistogramMergerthat if the result is queried, the merger is cleared and the caller takes ownership of the returned histogram. Therefore the caller is then responsible for correctly callingclose()on the returned histogram.I was hesitant of doing it this way, because when implementing the ES|QL support this might cause unnecessary allocations:
E.g. for a query like
| STATS PERCENTILE(MERGE(responseTime), 0.9) by service.name, TBUCKET(1m)it might be more efficient if we can reuse theExponentialHistogramMergeracross the aggregation groups. In this case this would mean merging all histograms for the firstservice.name, query the percentile and then continue with the next group. It would be ideal if thePERCENTILEcould operate on a histogram instance "lent" from the merger, so that it can be reused then for the next group instead of requiring a new allocation.However, if this is actually useful we can add this behaviour later when we get to the ES|QL implementation.